Skip to content

winit-core: new crate - #4208

Merged
kchibisov merged 18 commits into
masterfrom
kchibisov/split-core
May 14, 2025
Merged

winit-core: new crate#4208
kchibisov merged 18 commits into
masterfrom
kchibisov/split-core

Conversation

@kchibisov

@kchibisov kchibisov commented Apr 29, 2025 •
edited by madsmtm
Loading

Copy link
Copy Markdown
Member

Split trait based public API of winit into winit-core. Part of #3433.

--

Draft for now, since need to take a pass on docs and organize where things should lay.

madsmtm
madsmtm reviewed Apr 29, 2025
View reviewed changes

@madsmtm madsmtm left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick look so far, very focused on making reviewing this easier.

Comment thread winit-core/build.rs
Comment thread src/event_loop.rs
Comment thread examples/window.rs Outdated
Comment thread src/platform_impl/apple/appkit/view.rs Outdated
@madsmtm madsmtm added this to the Version 0.31.0 milestone Apr 29, 2025
@kchibisov
kchibisov force-pushed the kchibisov/split-core branch 9 times, most recently from 12e2587 to 42c9a07 Compare May 3, 2025 13:17
@kchibisov
kchibisov marked this pull request as ready for review May 3, 2025 13:39
@kchibisov
kchibisov requested a review from madsmtm May 3, 2025 13:39
@kchibisov

kchibisov commented May 3, 2025

Copy link
Copy Markdown
Member Author

I've split it somehow logically. Also, I haven't added everything to public API since I made a bunch of constructors pub. Though, they are not that relevant for the users, since they can not do much with them.

I'll still go through and add missing.

Also, not sure what to do with platform specific docs in winit-core.

@kchibisov

kchibisov commented May 3, 2025

Copy link
Copy Markdown
Member Author

Also, the WindowAttributes and the platform part on it I'm not that found of, it does kind of what it should, but I feel like there could be a better way...

@kchibisov

kchibisov commented May 3, 2025

Copy link
Copy Markdown
Member Author

The API will also need non_exhaustive throughout, but I think that and other tuning of such sort are for the later PRs.

madsmtm
madsmtm requested changes May 4, 2025
View reviewed changes

@madsmtm madsmtm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed it commit-by-commit, and then globally, it was very clear to follow. Though I think we should probably still squash-merge, for better git history (again, very much my focus here).

Requesting changes to:

  • Figure out the docs stuff.
  • Perhaps move a few things to separate PRs.
  • Discuss avoiding constructors on POD types like VideoMode.

Comment thread src/error.rs Outdated
Comment thread .github/workflows/ci.yml
Comment thread winit-core/src/monitor.rs
Comment thread winit-core/src/window.rs
Comment thread winit-core/src/application/mod.rs
Comment thread winit-core/src/application/mod.rs
Comment thread winit-core/src/event_loop.rs
Comment thread winit-core/src/lib.rs
Comment thread winit-core/src/window.rs Outdated
Comment thread winit-core/src/window.rs
@kchibisov

kchibisov commented May 5, 2025

Copy link
Copy Markdown
Member Author

Also, the intend is not squash anything, since every commit builds on its own, so while I can take things out, some things don't make sense without the rest of changes here e.g. window attributes.

@kchibisov
kchibisov force-pushed the kchibisov/split-core branch from 42c9a07 to 99831d5 Compare May 5, 2025 12:35
@kchibisov
kchibisov requested a review from madsmtm May 5, 2025 12:39
@madsmtm madsmtm mentioned this pull request May 5, 2025
Merged
@madsmtm

madsmtm commented May 5, 2025

Copy link
Copy Markdown
Member

I'm okay with rebase-merging, but only if you squash "winit-core: move window, except attrs", "winit-core: move WindowAttributes" and "winit: drop window module stub". Again, I want git blame winit-core/src/window.rs to work.

@kchibisov
kchibisov force-pushed the kchibisov/split-core branch from 99831d5 to 355e500 Compare May 5, 2025 13:52
@kchibisov

kchibisov commented May 5, 2025

Copy link
Copy Markdown
Member Author

I'm okay with rebase-merging, but only if you squash "winit-core: move window, except attrs", "winit-core: move WindowAttributes" and "winit: drop window module stub". Again, I want git blame winit-core/src/window.rs to work.

squashed them.

madsmtm
madsmtm reviewed May 5, 2025
View reviewed changes
Comment thread .github/workflows/ci.yml Outdated
Comment thread src/event_loop.rs
@madsmtm

madsmtm commented May 5, 2025

Copy link
Copy Markdown
Member

The only real blocker for me now is that the EventLoop stuff is a fair bit of git blame that will be lost, and that'd be unfortunate if the plan is to move it to winit-core anyhow at a later date.

@madsmtm madsmtm mentioned this pull request May 12, 2025
Open
madsmtm
madsmtm approved these changes May 13, 2025
View reviewed changes

@madsmtm madsmtm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#4208 (comment) is still open, but otherwise I'm good to move forwards with this.

Comment thread winit-core/src/event.rs Outdated
Comment thread src/platform_impl/linux/wayland/window/mod.rs
@madsmtm madsmtm added the S - api Design and usability label May 13, 2025
@kchibisov
kchibisov force-pushed the kchibisov/split-core branch from 355e500 to 09038bc Compare May 14, 2025 12:09
kchibisov added 18 commits May 14, 2025 21:09
@kchibisov
ci/deny: allow scripts in zerocopy
09c551b
@kchibisov
winit-core: new crate + split out as_any
3c3e3e6
@kchibisov
winit-core: move monitor handle
641be83
@kchibisov
winit-core: move keyboard
2b4c9ab
@kchibisov
winit-core: move icon
6187443
@kchibisov
winit-core: move cursor
7627054
@kchibisov
winit-core: move error
291a7bd
@kchibisov
winit-core: partially split event_loop
c695f77
@kchibisov
winit-core: move window
001076c 
Create `WindowAttributes` for respective platform specific window
attributes in `winit` due to move of `WindowAttributes`.
@kchibisov
winit-core: move ActiveEventLoop
ec4336b
@kchibisov
winit-core: move event
1d754dd
@kchibisov
winit-core: move application
c387cde
@kchibisov
winit-core: cleanup event loop docs
51bcecc
@kchibisov
winit-core: drop broken docs
17089fb 
Generally, winit-core doesn't know about underlying platforms, though,
some general information which will true for any implementation was
left in place.
@kchibisov
winit-core: drop all cfg except web
e0d2f98
@kchibisov
winit-core: fix tests
5c66cd3
@kchibisov
ci: test winit-core
4eb5203
@kchibisov
winit-core: add top-level doc
f320fb2
@kchibisov
kchibisov force-pushed the kchibisov/split-core branch from 09038bc to f320fb2 Compare May 14, 2025 12:10
@kchibisov
kchibisov merged commit b5921d8 into master May 14, 2025
@kchibisov
kchibisov deleted the kchibisov/split-core branch May 14, 2025 12:18
@madsmtm

madsmtm commented May 14, 2025

Copy link
Copy Markdown
Member

Thanks for pushing forwards on this @kchibisov!

@madsmtm madsmtm mentioned this pull request May 15, 2025
Closed
5 tasks
@madsmtm madsmtm mentioned this pull request Jun 5, 2025
Merged
@madsmtm madsmtm mentioned this pull request Sep 4, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@madsmtm madsmtm madsmtm approved these changes
@jackpot51 jackpot51 Awaiting requested review from jackpot51
@daxpedda daxpedda Awaiting requested review from daxpedda
@MarijnS95 MarijnS95 Awaiting requested review from MarijnS95
+2 more reviewers
@PoignardAzur PoignardAzur PoignardAzur left review comments
@notgull notgull notgull approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

S - api Design and usability

Milestone

Version 0.31.0

Development

Successfully merging this pull request may close these issues.

4 participants

@kchibisov @madsmtm @PoignardAzur @notgull